Skip to content

panic_str only exists for the migration to 2021 panic macros #123050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 23, 2024
Merged

panic_str only exists for the migration to 2021 panic macros #123050

merged 2 commits into from
Apr 23, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 25, 2024 •
edited
Loading

The only caller is expect_failed, which is already a cold inline(never) function, so inlining into that function should be fine. (And indeed panic_str was #[inline] anyway.)

The existence of panic_str risks someone calling it when they should call panic instead, and I can't see a reason why this footgun should exist.

I also extended the comment in panic to explain why it needs a 'static string -- I know I've wondered about this in the past and it took me quite a while to understand.

@rustbot
Copy link
Collaborator

rustbot commented Mar 25, 2024

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 25, 2024
@RalfJung
Copy link
Member Author

r? @m-ou-se

@rustbot rustbot assigned m-ou-se and unassigned scottmcm Mar 25, 2024
@m-ou-se
Copy link
Member

m-ou-se commented Mar 25, 2024 •
edited
Loading

panic_str is also used here:

rust/library/core/src/panic.rs

Lines 30 to 33 in 60b5ca6

// Use `panic_str` instead of `panic_display::<&str>` for non_fmt_panic lint.
($msg:expr $(,)?) => ({
$crate::panicking::panic_str($msg);
}),

(See also the diagram in #116005)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

RalfJung commented Mar 25, 2024 via email

@RalfJung
Copy link
Member Author

It seems like that lint does have type information available via cx.typeck_results().expr_ty. So instead of checking for panic_str it could check for panic_display with an &&str argument -- or would that not give the right results?

@bors
Copy link
Collaborator

bors commented Mar 26, 2024

☔ The latest upstream changes (presumably #123065) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member Author

It seems like that lint does have type information available via cx.typeck_results().expr_ty. So instead of checking for panic_str it could check for panic_display with an &&str argument -- or would that not give the right results?

I guess that would not be right since the lint should kick in for println!(mystrvar) but not println!("{}", mystrvar).

@RalfJung RalfJung changed the title remove panic_str, inline into the only caller panic_str only exists for the migration to 2021 panic macros Mar 26, 2024
@RalfJung
Copy link
Member Author

@rustbot ready

@rust-log-analyzer

This comment has been minimized.

Sign in to view
RalfJung
RalfJung commented Mar 26, 2024
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

@m-ou-se I have updated the PR based on your comment. Do you want to take another look or should I try to find a different reviewer?

m-ou-se
m-ou-se approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! r=me when ready.

@RalfJung
Copy link
Member Author

This is ready, I just rebased to fix a typo in the commit message. :)

@bors r=m-ou-se rollup

@bors
Copy link
Collaborator

bors commented Apr 23, 2024

📌 Commit 132921e has been approved by m-ou-se

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 23, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Apr 23, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
@bors
Auto merge of rust-lang#124289 - matthiaskrgr:rollup-oxw52jy, r=matth…
ad07aa1 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#123050 (panic_str only exists for the migration to 2021 panic macros)
 - rust-lang#124067 (weak lang items are not allowed to be #[track_caller])
 - rust-lang#124099 (Disallow ambiguous attributes on expressions)
 - rust-lang#124284 (parser: remove unused(no reads) max_angle_bracket_count field)
 - rust-lang#124288 (remove `push_trait_bound_inner`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 72e8fb4 into rust-lang:master Apr 23, 2024
@rustbot rustbot added this to the 1.79.0 milestone Apr 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
@rust-timer
Unrolled build for rust-lang#123050
8e67ba9 
Rollup merge of rust-lang#123050 - RalfJung:panic_str, r=m-ou-se

panic_str only exists for the migration to 2021 panic macros

The only caller is `expect_failed`, which is already a cold inline(never) function, so inlining into that function should be fine. (And indeed `panic_str` was `#[inline]` anyway.)

The existence of panic_str risks someone calling it when they should call `panic` instead, and I can't see a reason why this footgun should exist.

I also extended the comment in `panic` to explain why it needs a `'static` string -- I know I've wondered about this in the past and it took me quite a while to understand.
@RalfJung RalfJung deleted the panic_str branch April 23, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@celinval celinval celinval left review comments

@m-ou-se m-ou-se m-ou-se approved these changes

Assignees

@m-ou-se m-ou-se

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.
7 participants
@RalfJung @rustbot @m-ou-se @rust-log-analyzer @bors @celinval @scottmcm